Add OAuth #879

cjavilla-stripe · 2019-06-21T17:43:18Z

Looking for some feedback:

Is this PR too big?
Is this a decent param passing approach? (I'm about 95% confident that I understand the reason for using pointers)
Is stubbing out the server like this ok to test the oauth flow since oauth isnt supported by stripe-mock?

Here's a sample echo server that uses this successfully:

package main
import (
  // "bytes"
  // "encoding/json"
  "github.com/foolin/goview/supports/echoview"
  "github.com/labstack/echo"
  "github.com/labstack/echo/middleware"
  "log"
  "net/http"
  "github.com/stripe/stripe-go"
  "github.com/stripe/stripe-go/oauth"
)

func main() {
  stripe.ClientID = "ca_123"
  stripe.Key = "sk_123"

  e := echo.New()
  e.Use(middleware.Logger())
  e.Use(middleware.Recover())
  e.Renderer = echoview.Default()

  e.GET("/", func(c echo.Context) error {
    return c.Render(http.StatusOK, "index", echo.Map{})
  })

  e.GET("/start", func(c echo.Context) error {
    // THEN...
    // client_id := "ca_123"
    // base := "https://connect.stripe.com"
    // path := "/oauth/authorize"
    // response_type := "code"
    // redirect_uri := "http://localhost:1323/callback"
    // query := fmt.Sprintf(
    //   "client_id=%s&response_type=%s&redirect_uri=%s",
    //   client_id,
    //   response_type,
    //   redirect_uri,
    // )
    // url := fmt.Sprintf("%s%s?%s", base, path, query)
    // NOW...
    url := oauth.AuthorizeURL(&stripe.AuthorizeURLParams{
      RedirectURI: stripe.String("http://localhost:1323/callback"),
    })
    return c.Redirect(http.StatusTemporaryRedirect, url)
  })

  e.GET("/callback", func(c echo.Context) error {
    // THEN...
    // client_secret := "sk_test_123"
    // code := c.QueryParam("code")
    // scope := c.QueryParam("scope")
    // state := c.QueryParam("state")
    // message := map[string]interface{}{
    //   "client_secret": client_secret,
    //   "grant_type": "authorization_code",
    //   "code": code,
    // }
    // bytesRepresentation, err := json.Marshal(message)
    // if err != nil {
    //   log.Fatalln(err)
    // }
    // resp, err := http.Post(
    //   "https://connect.stripe.com/oauth/token",
    //   "application/json",
    //   bytes.NewBuffer(bytesRepresentation),
    // )
    // if err != nil {
    //   log.Fatalln(err)
    // }
    // var result map[string]interface{}
    // json.NewDecoder(resp.Body).Decode(&result)
    // log.Println(result)

    token, err := oauth.New(&stripe.OAuthTokenParams{
      Code:      stripe.String(c.QueryParam("code")),
    })
    if err != nil {
      log.Fatalln(err)
    }
    return c.HTML(http.StatusOK, token.StripeUserID)
  })
  e.Logger.Fatal(e.Start(":1323"))
}

remi-stripe

Thanks a lot for taking this on already CJ!

I left some high level comments! Since this is something I'm less familiar with though I will defer to Brandur on doing the thorough review

oauth.go

oauth/client.go

oauth.go

nirajjayant · 2019-07-01T15:55:04Z

Excited to get to use this soon! Is there a rough timeline for when this can get in?

cjavilla-stripe · 2019-07-10T15:57:24Z

Hmm. tests are passing locally... Investigating

cjavilla-stripe · 2019-07-10T16:16:20Z

Odd. I didn't have golint installed and make test didn't yell or anything. Once I installed golint it was clear what needed to be fixed. I should've looked at the Travis output before that last comment :)

brandur-stripe · 2019-07-10T16:41:02Z

Odd. I didn't have golint installed and make test didn't yell or anything.

Lint's in Make, but it's a separate step (make lint). If you want to run it, I'd suggest just doing a make, which will run all, and run all the checks that we've bundled in there.

brandur-stripe

Awesome @cjavilla-stripe!! This is a feature that we've wanted for a long time, but been too lazy to do :) Thank you.

I left quite a few comments here, but in general it's all pretty cosmetic stuff with quite a few duplicates, so it shouldn't be anywhere near as bad as it seems.

Sorry about the delay on review — I think Remi and I both assumed the other person was going to do it, haha. Once you've made changes, feel free to bump it back to me.

ptal @cjavilla-stripe

client/api.go

brandur-stripe · 2019-07-17T18:33:37Z

form/form.go

@@ -498,6 +499,21 @@ func (f *Values) Encode() string {
 	return buf.String()
 }

+// EncodeValues encodes only the values into “URL encoded” form
+// ("bar=baz&foo=quux") so that we can have keys like `stripe_user[email]=test`
+func (f *Values) EncodeValues() string {


I don't think we should have two methods that do so close to the same thing here.

A couple suggestions:

Did you double-check what happens if you change Encode to not QueryEscape the key? I can't remember if there's a good reason that we this off hand.

Otherwise, we can just do the QueryEscape and then change the encoded square backets back to square brackets. This is what stripe-ruby does:

def self.url_encode(key) CGI.escape(key.to_s). # Don't use strict form encoding by changing the square bracket control # characters back to their literals. This is fine by the server, and # makes these parameter strings easier to read. gsub("%5B", "[").gsub("%5D", "]") end

I thought that not escaping the key would be considered a breaking change, so opted initially for a second encode method, but I think we control the keys here so not escaping the key seems fine.

Solved it this way. seems to work well :)

oauth.go

brandur-stripe · 2019-07-17T20:26:18Z

oauth/client.go

+	}
+	if stripe.StringValue(params.GrantType) == "" {
+		params.GrantType = stripe.String("authorization_code")
+	}


OOC, is there some particular rationale behind providing a parameter default here?

It may be a little more convenient I suppose, but I think our normal convention is just allowing the API request to fail the first time and signal back that grant_type=authorization_code should have been passed, at which point the user just corrects their implementation. It's one extra step, but they probably won't ever make the mistake again.

Yeah it was all about convenience. Trying to make it so that users of this can pass some basics and we send sensible defaults.

oauth/client.go

oauth/client_test.go

oauth/client.go

cjavilla-stripe · 2019-07-18T17:34:20Z

Thanks for the review and your patience, @brandur-stripe :)

form/form.go

cjavilla-stripe · 2019-07-18T17:56:24Z

Excited to get to use this soon! Is there a rough timeline for when this can get in?

@nirajjayantbolt hopefully soon! sorry for the delay 😄

stripe.go

oauth/client.go

stripe.go

brandur-stripe · 2019-07-18T19:40:43Z

stripe.go

+// ClientID is the Stripe Client ID used by default for OAuth requests.
+// Relevant OAuth parameter types can also be initialized with a specific
+// ClientID that will take precidence over this global ClientID.
+var ClientID string


@remi-stripe Any opinion on having a global ClientID versus just requiring that one be passed with parameters? I don't feel too strongly about it, but in general would prefer to avoid globals where possible, so I'd lean toward the latter.

We chatted via slack and he agreed with you, so I've removed :)

brandur-stripe · 2019-07-18T19:57:46Z

Oops, and realized I forgot to answer your original questions:

Is this PR too big?

Definitely big-ish, but most of our OAuth implementations in other languages came in as part of one big PR, so it's not that unusual.

Is this a decent param passing approach? (I'm about 95% confident that I understand the reason for using pointers)

Yeah, it was best to just follow the convention and use pointers, so you did the right thing.

The reason for pointer is that because variables in Go by default get their zero value (so a string defaults to "", a boolean defaults to false, etc.), without the pointer it's not possible to distinguish between a field that hasn't been set versus one that's been set explicitly to the zero value. For example:

type MyParams struct {
    MyStr string
}

// These were initialized quite differently, but they look
// identical in Go because `MyStr` defaults to ""
p1 := &MyParams{}
p2 := &MyParams{MyStr: ""}

This doesn't make that much of a difference a lot of the time, but there are a couple places where the zero values are important. For example, we might want to explicitly set a string in the API to an empty string to empty it, or a boolean in the API to a false value, or an integer in the API to 0. Before in stripe-go, this was only possible through some pretty gross workarounds, so we ended up switching to pointers everywhere.

The pointers are all nil when unset, and we can easily distinguish that from one that hasn't been set versus one that's been set with an empty value like stripe.String("").

Is stubbing out the server like this ok to test the oauth flow since oauth isnt supported by stripe-mock?

Yeah, what you did there seems sane to me. Thanks for testing!

brandur-stripe · 2019-07-18T20:06:37Z

oauth.go

+	Currency           *string `form:"currency"`
+	DOBDay             *uint64 `form:"dob_day"`
+	DOBMonth           *uint64 `form:"dob_month"`
+	DOBYear            *uint64 `form:"dob_year"`


And oops, actually if you wouldn't mind, these should be *int64 actually. They can't be negative so technically uint64 would be a better fit, but at one point we just decided to just go with int64 everywhere by convention — in practice we'll never get to numbers high enough that we need the extra range uint64 gets us, and using int64 gives us some forward compatibility in case a field that currently doesn't allow negative numbers allows them in the future.

oauth/client_test.go

cjavilla-stripe · 2019-07-19T02:16:27Z

Thanks for that explanation about our pointer use, @brandur-stripe. Remi also shared that with me and seeing those examples really drives it home.

I think there are only two major things that need review:

error handling - My current approach is to (1) attempt to deserialize json with the current struct object (top level error key pointing at error data, which is the common error shape), if that fails (2) attempt to deserialize with the top level Error struct type which has 2 new fields for OAuth specific errors. (https://github.com/stripe/stripe-go/pull/879/files#diff-faeee970b41138d8bdc51e7aa3601141)
global client ID - @remi-stripe thoughts on global vs requiring that at call time? (https://github.com/stripe/stripe-go/pull/879/files#diff-ef7561d9a44d35bccb615dd5adb151be)

brandur-stripe · 2019-07-19T14:07:50Z

My current approach is to (1) attempt to deserialize json with the current struct object (top level error key pointing at error data, which is the common error shape), if that fails (2) attempt to deserialize with the top level Error struct type which has 2 new fields for OAuth specific errors.

Thanks for changing! I was thinking about it some more, and I think this is definitely closer to the path we want. Having a single unified error object for API or OAuth is how it works in stripe-dotnet already [1], and how it's being proposed for stripe-ruby as well [2].

I think it's a little janky that we have to do that two-phase fallback for deserializing an error object (although definitely not the worse thing, and the comment helps a lot). Did you consider alternative a custom UnmarshalJSON for the Error type? We might be able to have it try to detect an API error versus OAuth error, and then pick one path or the other based off of type.

Anyway, sorry that this is turning into more work than you probably expected :) The good news is that I think we're pretty close at this point.

[1] https://github.com/stripe/stripe-dotnet/blob/master/src/Stripe.net/Entities/StripeError.cs#L88-L90
[2] https://github.com/stripe/stripe-ruby/pull/811/files#diff-30e2a416d16a9b8714284f021b0a68e0R51

brandur-stripe · 2019-07-19T14:10:10Z

And maybe the one thing we might still want to review is the use of default values for unspecified parameters in OAuth requests. I don't think we do this for any other object types, so my preference would be to lean against the pattern even if there may be some usability benefit.

One pitfall that I can see possibly happening is that the user doesn't have control anymore over whether the parameter is sent. Say they purposely didn't want to send a particular parameter, they now no longer have a great way to do that (they could do stripe.String(""), but I don't think we should make them resort to that).

cjavilla-stripe · 2019-07-25T23:27:18Z

Removed the remaining defaults for unspecified values, removed the global ClientID, and took another swag at unmarshaling the error data by forking based on the BackendImplementation type.

r? @brandur-stripe

brandur-stripe · 2019-07-26T18:32:44Z

client/api.go

 	a.OrderReturns = &orderreturn.Client{B: backends.API, Key: key}
+	a.Orders = &order.Client{B: backends.API, Key: key}


Thanks for fixing :)

brandur-stripe · 2019-07-26T18:53:32Z

error.go

+
+	// OAuth specific Error properties. Named OAuthError because of name conflict.
+	OAuthError            string `json:"error,omitempty"`
+	OAuthErrorDescription string `json:"error_description,omitempty"`


I actually like the "OAuth" prefix anyway — it makes it harder to use these outside of an OAuth context by accident (it's clear they're specifically for OAuth and nothing else).

This was actually required because we use Error as a method on this object elsewhere.

brandur-stripe · 2019-07-26T18:55:30Z

oauth/client_test.go

+			State:              stripe.String("NV"),
+			StreetAddress:      stripe.String("123 main"),
+			URL:                stripe.String("http://example.com"),
+			Zip:                stripe.String("12345"),


I just realized that "Zip" is actually an acronym (Zone Improvement Plan) and probably should've been "ZIP" instead. Don't change it though because we've already got the "Zip" convention in some fields elsewhere.

Got it. Yeah tried to follow convention :)

stripe.go

brandur-stripe · 2019-07-26T18:58:35Z

@cjavilla-stripe New changes look great! Would you mind squashing this down? After that I think we're pretty much good to go.

@remi-stripe This looks ready to me. Do you want to take a pass for yourself before we go to release?

cjavilla-stripe

Squashed and made those final minor adjustments.

brandur-stripe

ptal @cjavilla-stripe Just a couple last-minute comments here, but after those are fixed I think we're good to ship this. Thanks!

oauth/client_test.go

brandur-stripe · 2019-07-29T22:46:19Z

oauth/client_test.go

+// RoundTripFunc.
+type RoundTripFunc func(req *http.Request) *http.Response
+
+// RoundTrip.


And same with this one.

Unexported.

brandur-stripe · 2019-07-29T22:51:16Z

oauth/client_test.go

+func TestNewOAuthToken(t *testing.T) {
+	stripe.Key = "sk_123"
+
+	// stripe-mock doesn't support connect URL's so this stubs out the server.


Maybe consider changing all instances of "URL's" to "URLs"? I Googled and there's no clear answer for the correct way to handle these (sources vary), but my feeling is that the apostrophe-less version is generally preferred.

Updated these instances.

cjavilla-stripe · 2019-07-30T00:03:23Z

Thanks for the thorough review, @brandur-stripe! :)

brandur-stripe · 2019-07-30T00:04:28Z

Excellent!! Thank you again CJ!

brandur-stripe · 2019-07-30T00:06:29Z

Released as 61.23.0.

cjavilla-stripe · 2019-07-30T00:06:56Z

🎉

stripe-ci assigned remi-stripe Jun 21, 2019

remi-stripe reviewed Jun 21, 2019

View reviewed changes

oauth.go Outdated Show resolved Hide resolved

oauth.go Outdated Show resolved Hide resolved

oauth.go Outdated Show resolved Hide resolved

oauth.go Outdated Show resolved Hide resolved

oauth/client.go Outdated Show resolved Hide resolved

oauth.go Show resolved Hide resolved

remi-stripe assigned ob-stripe and brandur-stripe and unassigned remi-stripe Jun 21, 2019

cjavilla-stripe force-pushed the cjavilla/add-oauth branch 2 times, most recently from 15379aa to b396ce7 Compare June 25, 2019 17:52

cjavilla-stripe changed the title ~~[wip] Add OAuth~~ Add OAuth Jul 8, 2019

cjavilla-stripe force-pushed the cjavilla/add-oauth branch from ca17387 to 3c92581 Compare July 8, 2019 16:06

cjavilla-stripe force-pushed the cjavilla/add-oauth branch from 556fd8d to ca72804 Compare July 10, 2019 21:55

brandur-stripe reviewed Jul 17, 2019

View reviewed changes

stripe-ci assigned cjavilla-stripe and unassigned ob-stripe and brandur-stripe Jul 17, 2019

cjavilla-stripe force-pushed the cjavilla/add-oauth branch from ca72804 to d9ab7ec Compare July 18, 2019 17:33

cjavilla-stripe commented Jul 18, 2019

View reviewed changes

form/form.go Outdated Show resolved Hide resolved

brandur-stripe reviewed Jul 18, 2019

View reviewed changes

stripe.go Show resolved Hide resolved

brandur-stripe reviewed Jul 18, 2019

View reviewed changes

oauth/client.go Outdated Show resolved Hide resolved

brandur-stripe reviewed Jul 18, 2019

View reviewed changes

stripe.go Outdated Show resolved Hide resolved

brandur-stripe reviewed Jul 18, 2019

View reviewed changes

oauth/client_test.go Outdated Show resolved Hide resolved

cjavilla-stripe force-pushed the cjavilla/add-oauth branch from a0e49ad to 6f40d03 Compare July 19, 2019 01:38

cjavilla-stripe force-pushed the cjavilla/add-oauth branch from e9b2c0b to 9f0d44d Compare July 25, 2019 23:15

cjavilla-stripe assigned brandur-stripe and unassigned cjavilla-stripe Jul 25, 2019

brandur-stripe reviewed Jul 26, 2019

View reviewed changes

brandur-stripe mentioned this pull request Jul 26, 2019

Add ErrorObject to StripeError exceptions stripe/stripe-ruby#811

Merged

brandur-stripe reviewed Jul 26, 2019

View reviewed changes

stripe.go Show resolved Hide resolved

brandur-stripe reviewed Jul 26, 2019

View reviewed changes

stripe.go Outdated Show resolved Hide resolved

cjavilla-stripe force-pushed the cjavilla/add-oauth branch 2 times, most recently from bb8e843 to 3046fbf Compare July 27, 2019 00:51

cjavilla-stripe commented Jul 27, 2019

View reviewed changes

brandur-stripe reviewed Jul 29, 2019

View reviewed changes

stripe-ci assigned cjavilla-stripe and unassigned brandur-stripe Jul 29, 2019

Add OAuth support

f796999

cjavilla-stripe force-pushed the cjavilla/add-oauth branch from 3046fbf to f796999 Compare July 29, 2019 23:55

cjavilla-stripe assigned brandur-stripe and unassigned cjavilla-stripe Jul 30, 2019

brandur-stripe merged commit 4078afc into master Jul 30, 2019

brandur-stripe deleted the cjavilla/add-oauth branch July 30, 2019 00:04

		a.OrderReturns = &orderreturn.Client{B: backends.API, Key: key}
		a.Orders = &order.Client{B: backends.API, Key: key}

Add OAuth #879

Add OAuth #879

Conversation

cjavilla-stripe commented Jun 21, 2019

remi-stripe left a comment

Choose a reason for hiding this comment

nirajjayant commented Jul 1, 2019

cjavilla-stripe commented Jul 10, 2019

cjavilla-stripe commented Jul 10, 2019

brandur-stripe commented Jul 10, 2019

brandur-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjavilla-stripe commented Jul 18, 2019

cjavilla-stripe commented Jul 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandur-stripe commented Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

cjavilla-stripe commented Jul 19, 2019 • edited Loading

brandur-stripe commented Jul 19, 2019

brandur-stripe commented Jul 19, 2019 • edited Loading

cjavilla-stripe commented Jul 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandur-stripe commented Jul 26, 2019

cjavilla-stripe left a comment

Choose a reason for hiding this comment

brandur-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjavilla-stripe commented Jul 30, 2019

brandur-stripe commented Jul 30, 2019

brandur-stripe commented Jul 30, 2019

cjavilla-stripe commented Jul 30, 2019

brandur-stripe commented Jul 18, 2019 •

edited

Loading

cjavilla-stripe commented Jul 19, 2019 •

edited

Loading

brandur-stripe commented Jul 19, 2019 •

edited

Loading